Skip to content

compressor: simplify logic#10447

Merged
mattklein123 merged 3 commits intoenvoyproxy:masterfrom
rgs1:simplify-compressor-filter
Mar 20, 2020
Merged

compressor: simplify logic#10447
mattklein123 merged 3 commits intoenvoyproxy:masterfrom
rgs1:simplify-compressor-filter

Conversation

@rgs1
Copy link
Member

@rgs1 rgs1 commented Mar 19, 2020

This confused me the first time I read it, so make it simpler
for the next reader -- there's no need to handle not_compressed_.inc()
from the two different places. Delay it until encodeHeaders() and
do it once.

Signed-off-by: Raul Gutierrez Segales rgs@pinterest.com

compressor: simplify logic
abc262a 
This confused me the first time I read it, so make it simpler
for the next reader -- there's no need to handle `not_compressed_.inc()`
from the two different places. Delay it until encodeHeaders() and
do it once.

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
@rgs1 rgs1 requested a review from dio as a code owner March 19, 2020 00:29
@rgs1
Copy link
Member Author

rgs1 commented Mar 19, 2020

@mattklein123 @dio take 2 :-)

@rgs1 rgs1 mentioned this pull request Mar 19, 2020
Closed
Simplify a bit more
e551346 
The skip_compression_ is being conflated with the the enabled check, so
split that apart so it's easier to read.

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
@rgs1
Copy link
Member Author

rgs1 commented Mar 19, 2020

Hmm, failed test passes locally for me:

//test/extensions/filters/http/cache:cache_filter_integration_test       PASSED in 4.3s

So guessing transient:

test/extensions/filters/http/cache/cache_filter_integration_test.cc:68
Value of: request->headers()
Expected: is a superset of headers:
':status', '200'
'date', 'Thu, 01 Jan 1970 01:00:01 GMT'
'cache-control', 'public,max-age=3600'
'content-length', '42'

  Actual: 16-byte object <E0-AB 68-01 00-00 00-00 B0-B3 68-01 00-00 00-00> (of type Envoy::Http::ResponseHeaderMap), where the following matchers don't match any elements:
matcher #1: is equal to ("date", "Thu, 01 Jan 1970 01:00:01 GMT")
Stack trace:
  0x84d18c: Envoy::Extensions::HttpFilters::Cache::CacheIntegrationTest_MissInsertHit_Test::TestBody()
  0x15f1028: testing::internal::HandleExceptionsInMethodIfSupported<>()
  0x15f0f55: testing::Test::Run()
  0x15f1e20: testing::TestInfo::Run()
... Google Test internal frames ...

cc: @toddmgreer

@dio
Copy link
Member

dio commented Mar 19, 2020

/azp run envoy-linux

@azure-pipelines
Copy link

azure-pipelines bot commented Mar 19, 2020

Azure Pipelines successfully started running 1 pipeline(s).

@dio
Copy link
Member

dio commented Mar 19, 2020

@rojkov could you help to take a look at this? Thanks!

rojkov
rojkov previously approved these changes Mar 19, 2020
View reviewed changes
Copy link
Member

@rojkov rojkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This is simpler indeed.

Tighten up the number of expected calls
f5e91ce 
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
rojkov
rojkov approved these changes Mar 20, 2020
View reviewed changes
Copy link
Member

@rojkov rojkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks!

mattklein123
mattklein123 approved these changes Mar 20, 2020
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mattklein123 mattklein123 merged commit b16ce6d into envoyproxy:master Mar 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

@dio dio Awaiting requested review from dio

+1 more reviewer

@rojkov rojkov rojkov approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@rojkov rojkov

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rgs1 @dio @rojkov @mattklein123